Skip to content

apply: extract internal/apply package; wire MCP apply / status / diagnose - #157

Merged
kuny0707 merged 1 commit into
tronprotocol:developfrom
barbatos2011:feat/extract-apply
May 2, 2026
Merged

kuny0707 merged 1 commit into
tronprotocol:developfrom
barbatos2011:feat/extract-apply

Conversation

@barbatos2011

Copy link
Copy Markdown

Summary

Extracts the deploy + probe primitives out of cmd/apply.go and cmd/status.go into a new internal/apply package. cmd/* becomes a thin presentation layer; MCP tools that previously returned NOT_IMPLEMENTED or state-only subsets now use the same in-process functions.

This unblocks items 2 + 3 from the post-quartet improvement list.

What ships

New package: internal/apply

File Purpose
apply.go Apply(ctx, Options) (*Result, error) — pure-function deploy: render → runtime → state save → optional wait. Includes hash-match short-circuit.
wait.go WaitForReady(ctx, target, name, port, timeout) — extracted from cmd/apply.go::waitForNodeReady
probe.go LiveStatus(ctx, target, node) — extracted from cmd/status.go::liveStatusProbe (block_height / is_synced / peer_count via docker-exec curl)
apply_test.go 4 tests: no-op short-circuit on hash match, missing-input validation, IntentHashFromBytes stability against stdlib SHA256

cmd/* shrinks

  • cmd/apply.go::runApply shrinks from ~220 → ~110 lines. Lock + HUMAN_REQUIRED gate + output formatting stay; deploy phase is one apply.Apply call. Backward-compatible JSON shape preserved (legacy status / changes keys still emitted).
  • cmd/status.go::liveStatusProbe shrinks to a 10-line wrapper that resolves the target then hands off.
  • cmd/plan.go's local sha256hex helper replaced with apply.IntentHashFromBytes.

MCP tools that gained real behavior

  • apply — was returning NOT_IMPLEMENTED_VIA_MCP; now actually deploys via apply.Apply. Test changed from asserting NOT_IMPLEMENTED to asserting validation works on a bogus path.
  • status — was returning state-only fields; now runs apply.LiveStatus when the node is running, surfacing block_height / is_synced / peer_count.
  • diagnose — was running a state-only subset; now runs the full diagnosis.AllCheckers() suite (sync_progress, peer_count, disk_space, port_listening, memory_usage, version) just like trond diagnose <name> does.

All three reuse mcpResolveTargetFromNode for SSH-aware target resolution + defer Close cleanup.

Test plan

  • make build && make test — every existing test still green; new internal/apply tests cover the no-op + validation paths
  • ./bin/trond apply --intent examples/nile-fullnode.yaml --dry-run — same shape as before refactor (no shape regression)
  • CI 8 jobs all green

Backwards compatibility

  • CLI: unchanged. JSON shape preserved by translating apply.Result back into the legacy status / changes / endpoints map at the cmd boundary.
  • MCP: three previously-broken tools become functional. No removals; no rename. Existing MCP clients calling status / diagnose see additional fields when the node is running.
  • internal/apply is new; nothing depends on it externally yet.

Lint config note

.golangci.yml: extended the cmd/* closer.Close errcheck exclusion to internal/mcp/* since the pattern is identical there (target.Target stored in interface{ Close() error }, deferred for cleanup).

🤖 Generated with Claude Code

…nose

Closes the three biggest "MCP returns NOT_IMPLEMENTED / state-only
subset" gaps from PR tronprotocol#153 by hoisting the deploy + probe primitives
out of cmd/apply.go and cmd/status.go into a new internal/apply
package. cmd/* becomes a thin presentation layer that wraps the same
calls MCP and recipe runners now make directly.

  internal/apply/apply.go   Apply(ctx, Options) (*Result, error)
                            — pure-function deploy: render → runtime
                            → state save → optional wait. Includes
                            no-op short-circuit on hash match.
  internal/apply/wait.go    WaitForReady(ctx, target, name, port,
                            timeout) — extracted from
                            cmd/apply.go::waitForNodeReady.
  internal/apply/probe.go   LiveStatus(ctx, target, node) — extracted
                            from cmd/status.go::liveStatusProbe.
                            Returns block_height / is_synced /
                            peer_count via docker-exec curl.
  internal/apply/apply_test.go   4 tests covering no-op short-circuit,
                            input validation, and IntentHashFromBytes
                            stability against the stdlib SHA256.

The cmd-side changes:
  - cmd/apply.go::runApply shrinks from ~220 lines to ~110; the
    deploy phase lives in apply.Apply, the cmd layer keeps lock /
    HUMAN_REQUIRED gate / output formatting. Backward-compatible
    JSON shape preserved (legacy "status" / "changes" keys still
    emitted).
  - cmd/status.go::liveStatusProbe shrinks to a 10-line wrapper
    that just resolves the target and hands off.
  - cmd/plan.go's local sha256hex helper replaced with
    apply.IntentHashFromBytes — same result, one source of truth.

The MCP-side wins (the whole point of the extraction):
  - apply tool now actually deploys instead of returning
    NOT_IMPLEMENTED_VIA_MCP. The previous test that asserted
    NOT_IMPLEMENTED is replaced with TestApply_RejectsBadIntentPath
    which exercises the validation path without needing Docker.
  - status tool now runs the live probe (block_height /
    is_synced / peer_count) when the node is running.
  - diagnose tool runs the full diagnosis.AllCheckers() suite
    instead of the previous state-only subset.
  - All three reuse the same target-resolution helper
    (mcpResolveTargetFromNode); SSH connection cleanup via defer.

.golangci.yml: extended the cmd/* closer-pattern errcheck exclusion
to internal/mcp/* (the pattern is identical: target.Target stored
in an `interface{ Close() error }` and deferred for cleanup).

Verified: every test green, lint exit 0. The cmd surface is
unchanged for users; the MCP surface gained three previously-stubbed
tools as fully working features.
barbatos2011 pushed a commit to barbatos2011/tron-deployment that referenced this pull request May 2, 2026
Three actionable findings from the post-implementation review:

(#1) MCP `apply` tool description was inherited from the stub days
("currently returns the inputs that would be passed to the CLI;
full in-process apply requires extracting cmd/apply.go's RunE...").
That's no longer true — PR tronprotocol#157's `applyTool` calls `apply.Apply`
directly. Updated the description to: "Idempotent in-process
deploy via internal/apply.Apply (the same pure function the CLI
uses)... wait failure leaves the deploy successful but reports
ready=false in the result."

(#2 false positive) Reviewer flagged `runChild` appending --output
json after `rollback <node>` as broken arg order. Verified via
real CLI invocation: cobra accepts interspersed flags by default,
`trond rollback bogus-node --output json` returns the expected
NODE_NOT_FOUND envelope. No fix needed; reviewer concern noted
for the record.

(tronprotocol#3) `internal/apply/apply.go::detectJDK` used inline atoiSafe /
hasPrefix / indexByte helpers "to keep imports trim." That's
penny-wise, pound-foolish: atoiSafe disagrees with strconv.Atoi on
edge cases (leading whitespace returns 0 instead of parsing past
it; negative inputs return 0 instead of the negative number).
Replaced with stdlib strings + strconv. The only behavioral change
is "more correct on edge cases that are unlikely but possible" —
a `java -version` output with leading whitespace would have
silently fallen back to JDK 17 before; now it parses correctly.

Test additions:
  - cmd/plan_test.go: TestSimpleHOCONDiff_DetectsLineChanges,
    EmptyWhenIdentical, HandlesUnequalLength + a smoke test for
    printDiffSection's three branches. Pure functions, easy wins.
  - internal/observability/otel_test.go: 8 cases covering schemeOf,
    stripScheme, parseTimeout (default / override / invalid),
    parseHeaders (empty / two entries / whitespace-trimmed /
    malformed-skipped), and TestInit_NoOpWhenEndpointUnset which
    confirms Init returns a non-nil shutdown function and the
    default no-op path doesn't error.

All tests pass; lint exit 0.
@kuny0707
kuny0707 merged commit 2698650 into tronprotocol:develop May 2, 2026
8 checks passed
kuny0707 pushed a commit that referenced this pull request May 2, 2026
Three follow-up improvements layered on top of the apply extraction
(PR #157), each enabled by a piece of that refactor.

(5) `trond plan --diff`
  - Surfaces the line-by-line HOCON content delta alongside the
    existing field-level changes[] array. Reviewers see WHICH config
    keys would actually change, not just that hashes drifted.
  - Sourced from the deployed config persisted under
    ~/.trond/deployments/<name>/<name>.conf vs the freshly-rendered
    HOCON; line-by-line via the same simpleDiff shape cmd/config/diff
    uses. JSON output gains "config_diff": [...] when --diff is set.

(6) `trond network upgrade <network>`
  - Rolling upgrade across every node in a private network.
    Default order: fullnodes first (one at a time), then witnesses
    (one at a time) so block production stays up during rollout.
    --witness-first inverts the order for cases where the operator
    wants witness ABI first.
  - Each node is upgrade'd then immediately verify'd; failure halts
    the rollout. With --auto-rollback, every already-upgraded node
    gets `trond rollback`'d before exit; without it, the operator is
    told which nodes need manual revert.
  - Each per-node step is a re-exec of the trond binary itself for
    clean lock semantics + independent logging. No shared in-process
    state across nodes.

(7) OpenTelemetry trace export
  - New internal/observability package with Init() / Tracer() /
    Shutdown contract. main.go calls Init at start, Shutdown before
    os.Exit so spans flush.
  - Default is no-op tracer (zero overhead, no network IO). Opt-in
    when TROND_OTLP_ENDPOINT is set; supports OTLP-over-HTTP with
    auto scheme detection (http://... → insecure, https://... →
    TLS), TROND_OTLP_HEADERS for auth, TROND_OTLP_TIMEOUT for
    shutdown flush budget (default 5s).
  - Failure to initialise the exporter falls back to no-op with a
    stderr warning, so missing collectors never break the user's
    actual trond invocation.

AGENTS.md: documents --diff under Workflow 1, the new
`network upgrade` step under Workflow 4, and TROND_OTLP_ENDPOINT in
the universal-calling-convention section.

Lint exit 0. All existing tests pass; no in-package tests added for
network upgrade (subprocess re-exec makes meaningful unit tests
require a fake trond binary fixture; deferred until the e2e suite
lands).
kuny0707 pushed a commit that referenced this pull request Jun 12, 2026
New `proto-drift` job in .github/workflows/ci.yml that re-runs
scripts/gen-dbfork-protos.sh and fails if internal/dbfork/proto/pb/
changes. Catches two regression classes:

1. Upstream .proto edit via git subtree pull without re-running the
   gen script. Committed Go bindings would silently lag the proto
   definitions and the engine would marshal against stale schemas.
2. Hand-edit of a *.pb.go file. The files look like ordinary Go
   and tempt operators to "just tweak" — but they're machine-
   generated and the next regen clobbers them.

The gate uses `arduino/setup-protoc@v3` for protoc + pins
protoc-gen-go to v1.36.11 (matching google.golang.org/protobuf in
go.mod). Mismatched generator vs runtime versions produce
cosmetically-different .pb.go output that would fail the diff for
the wrong reason — Task #157 will consolidate the pin into
tools.go so there's a single source of truth.

proto/README.md: docs the v1.36.11 pin + the new CI gate so future
contributors know which version to install + why the diff fails if
their version is off.

TDD-verified locally: introduced a sentinel comment in Tron.pb.go,
confirmed `git diff --exit-code` returns 1; restored, returns 0.
Regenerated pb/ with locally-installed v1.36.11 — output is
byte-identical to the committed bindings, so CI will start green
on the next push.
kuny0707 pushed a commit that referenced this pull request Jun 12, 2026
Replaces the duplicated v1.36.11 pin (CI yaml + proto README +
implicit-via-go.mod-runtime) with a single source of truth: the
Go 1.24+ `tool` directive in go.mod. Mismatched generator vs
runtime versions are now structurally impossible — both the
runtime (`require google.golang.org/protobuf v1.36.11`) and the
generator (`tool google.golang.org/protobuf/cmd/protoc-gen-go`)
resolve from the same go.mod entry.

Pieces:
- go.mod: `tool google.golang.org/protobuf/cmd/protoc-gen-go`
  added via `go get -tool`. No version literal duplicated
  anywhere — `go install tool` reads the pin from here.
- .github/workflows/ci.yml: proto-drift job's install step
  switches from `go install <pkg>@v1.36.11` to `go install tool`.
  Comment updated to explain the single-source-of-truth design.
- internal/dbfork/proto/README.md: tooling-install section drops
  the hardcoded version; uses `go install tool` for both macOS +
  Linux. The "if you see drift, your install is off" debugging
  hint is preserved.
- scripts/gen-dbfork-protos.sh: when protoc-gen-go is missing,
  the error message now suggests the exact install command
  (`go install tool`) instead of just pointing at the README.

TDD-verified locally: `go install tool` installs v1.36.11 (matches
go.mod's runtime version). Re-running the gen script produces
byte-identical pb/ output → drift check stays green. Test sweep +
lint + shellcheck all clean.

The CI yaml's pinned dep table is now exactly as long as it needs
to be: a Go version, a protoc version (different toolchain entirely),
and the actions used. The protoc-gen-go pin moved to where it
belongs — alongside its runtime dep in go.mod.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants